MLE-19601 : Update annTopK in Node API#937
Conversation
anu3990
requested review from
BillFarber,
Copilot and
rjrudin
and removed request for
Copilot
There was a problem hiding this comment.
Pull Request Overview
This PR extends the annTopK Node API to accept new PlanAnnTopKOptions in string, array, or map form and adds corresponding tests and builder logic.
- Added multiple tests in
test-basic/annTopK.jscovering absence of options, string, array, map, and invalid options. - Updated
plan-builder-generated.jsto mark theoptionsparameter as a named (optional) argument. - Enhanced
plan-builder-base.jsto validateMapinputs forPlanAnnTopKOptionsand extend string type handling.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test-basic/annTopK.js | New tests for various annTopK option formats; shared map usage |
| lib/plan-builder-generated.js | Changed annTopK paramdefs to allow named options argument |
| lib/plan-builder-base.js | Added cast logic for PlanAnnTopKOptions; extended string types |
Comments suppressed due to low confidence (1)
test-basic/annTopK.js
Outdated
| const testlib = require('../etc/test-lib'); | ||
| let serverConfiguration = {}; | ||
| const execPlan = pbb.execPlan; | ||
| const planAnnTopKOptionsMap = new Map(); |
There was a problem hiding this comment.
Declaring a shared Map at file scope can lead to state leakage across tests. Consider initializing a fresh Map within each it(...) or using beforeEach to reset it.
| const planAnnTopKOptionsMap = new Map(); | |
| // Removed global declaration of planAnnTopKOptionsMap. |
There was a problem hiding this comment.
This sounds like a good change to make, it's better to declare it in each test as needed.
test-basic/annTopK.js
Outdated
| assert(rows[1].distance.type === 'xs:float', 'Verifying that the distance column was populated.'); | ||
| done(); | ||
| } catch (error){ | ||
| throw error; |
There was a problem hiding this comment.
Throwing inside the verifyResults catch won’t invoke the Mocha done callback on failure. Replace throw error with done(error) to correctly signal test failures.
| throw error; | |
| done(error); |
There was a problem hiding this comment.
This sounds like a good change too?
| const planAnnTopKOptionsSet = new Set(['maxDistance', 'max-distance', 'searchFactor','search-factor']); | ||
| if(Object.getPrototypeOf(arg) === Map.prototype){ | ||
| arg.forEach((value, key) => { | ||
| if(!planAnnTopKOptionsSet.has(key)) { | ||
| throw new Error( | ||
| `${argLabel(funcName, paramName, argPos)} has invalid key- ${key}` | ||
| ); | ||
| } | ||
| }); |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
rjrudin
left a comment
rjrudin
left a comment
There was a problem hiding this comment.
Looks good, I just agreed with a couple Copilot suggestions for small changes to the test.
test-basic/annTopK.js
Outdated
| const testlib = require('../etc/test-lib'); | ||
| let serverConfiguration = {}; | ||
| const execPlan = pbb.execPlan; | ||
| const planAnnTopKOptionsMap = new Map(); |
There was a problem hiding this comment.
This sounds like a good change to make, it's better to declare it in each test as needed.
test-basic/annTopK.js
Outdated
| assert(rows[1].distance.type === 'xs:float', 'Verifying that the distance column was populated.'); | ||
| done(); | ||
| } catch (error){ | ||
| throw error; |
There was a problem hiding this comment.
This sounds like a good change too?
No description provided.